Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(Sharding): strict type context and return #5933

Merged
merged 2 commits into from
Jun 29, 2021
Merged

fix(Sharding): strict type context and return #5933

merged 2 commits into from
Jun 29, 2021

Conversation

kyranet
Copy link
Member

@kyranet kyranet commented Jun 26, 2021

Please describe the changes this PR makes and why it should be merged:

Right now, ShardClientUtil#broadcastEval and ShardingManager#broadcastEval's return types are the same as the callback's, as well as the context passed, however, the context data is serialized, and so is the return type.

This PR implements Serialized, which fixes this issue by converting rich types into their serialized counterpart.

Status and versioning classification:

  • I know how to update typings and have done so, or typings don't need updating

@kyranet kyranet force-pushed the fix/strict-type-broadcast-eval branch from 5fc19a7 to 37a9140 Compare June 26, 2021 22:08
@kyranet
Copy link
Member Author

kyranet commented Jun 26, 2021

I'm unsure how to properly type the returns because of the worker mode:

discord.js/src/sharding/ShardClientUtil.js

Lines 92 to 95 in 985d4d6

} else if (this.mode === 'worker') {
this.parentPort.postMessage(message);
resolve();
}

process.send seems to JSON serialize values, however, the worker mode supports sending a lot richer data, as this is powered by V8's serder instead.

@iCrawl
Copy link
Member

iCrawl commented Jun 27, 2021

This needs a rebase.

@iCrawl iCrawl added this to the Version 13 milestone Jun 27, 2021
@kyranet kyranet force-pushed the fix/strict-type-broadcast-eval branch from 37a9140 to 669b170 Compare June 28, 2021 10:33
@kyranet kyranet force-pushed the fix/strict-type-broadcast-eval branch from 669b170 to 306cc90 Compare June 28, 2021 10:35
@iCrawl
Copy link
Member

iCrawl commented Jun 28, 2021

Can't we just go with this for now, despite the worker mode and potentially wait for an issue in case anyone is using this to send more complicated data?

@LosTigeros
Copy link

LosTigeros commented Jun 28, 2021 •
edited
Loading

What about executing functions that would return a Promise on the shard?
Can't this be like:

promises.push(Promise.resolve(sh[method](...args)));

This would work for normal functions and PromiseLike ones so broadcastEval script/fn argument could be either T or Promise<T>

@kyranet
fix: unwrap promises
52ec31f 
Also added tests to make sure they work
@kyranet
Copy link
Member Author

kyranet commented Jun 28, 2021

Yes, we can go for this for now, and if we wanted to make both consistent, we can always use V8's serialization API, which is what worker threads use, @iCrawl.

Nice catch @LosTigeros! I changed the return of the callbacks from T to Awaited<T>, so in case it's a promise, the promise's type is unwrapped. Also added tests to make sure they work as expected, see 52ec31f :)

@iCrawl iCrawl merged commit 1925d01 into discordjs:master Jun 29, 2021
@kyranet kyranet deleted the fix/strict-type-broadcast-eval branch July 5, 2021 12:55
@SpaceEEC SpaceEEC mentioned this pull request Nov 20, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SpaceEEC SpaceEEC SpaceEEC approved these changes

@iCrawl iCrawl iCrawl approved these changes

@vladfrangu vladfrangu vladfrangu approved these changes

@amishshah amishshah amishshah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Version 13
Development

Successfully merging this pull request may close these issues.
6 participants
@kyranet @iCrawl @LosTigeros @amishshah @vladfrangu @SpaceEEC